New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add fromSurface option #8496
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
This is great. |
f92e9e3
to
20b6903
Compare
Thanks for the PR. Please provides tests for this. |
20b6903
to
8fa9940
Compare
Added the test. |
25dae4f
to
01aed73
Compare
01aed73
to
fbfb51d
Compare
@jrandolf Should I include the fix in this PR or in a separate PR? I'm afraid that because my workaround requires getting width and height from screenshot, an image-parsing library should be added - e.g. either jpeg-js or pngjs should be moved from devDependencies to dependencies... Would that be OK? |
I think we would not want to bring an image parsing library into the list of our prod dependencies. Could you please describe the latest problem you are facing? IIUC, the screenshot is bigger than the current viewport because setting the viewport does not change the window bounds? P.S. also if you could rebase once more, I would run the test on the bots |
The problem is that capturing Chromium screenshot in
Done. |
(just for me to understand it) so once we capture a screenshot with |
In my workaround Browser.setWindowBounds is used to first set the "full" window bounds to the viewport, then a |
In headless mode the option is not supported at all, so it is better to set the default to `undefined` instead of `true`
Checked the following methods of getting true bounds:
So the capturing of the screenshot appears to be the only option. |
Does the method |
It also returns full window bounds (i.e. with UI). |
Thanks for the PR! Let's land it and we can see if we find the workaround for the original issue. |
It seems that the test does not even work because of different UI sizes on different platforms, I think? Not sure... Also not sure about chrome-headless mode (does the options work in it or only in true headful), will investigate... |
Yes, exactly - it works perfectly for me on Windows. |
It does not support the option. So everything in this PR seems to be correct :-) |
@LeviPesin let's land the PR but update the test to just verify that a screenshot is taken. The dimensions of the screenshot are not really under control of Puppeteer so I think the solution to that can be found later. |
Done. |
let's also remove the golden image file! |
Done. |
What kind of change does this PR introduce?
Feature.
Did you add tests for your changes?
No, I am unsure how to do that. Also, it seems like there is no test for the
captureBeyondViewport
option.If relevant, did you update the documentation?
Yes.
Summary
This PR adds the fromSurface option to the
Page.screenshot
method.This option can e.g. be used to properly screenshot WebGPU (see mrdoob/three.js#24109 for a discussion).
Does this PR introduce a breaking change?
No.